Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/delivery alerts #768

Merged
merged 17 commits into from
Sep 26, 2023
Merged

Feature/delivery alerts #768

merged 17 commits into from
Sep 26, 2023

Conversation

aaronchongth
Copy link
Member

@aaronchongth aaronchongth commented Sep 12, 2023

What's new

  • delivery_alerts backend
  • frontend

Assumptions

  • When a delivery alert has tier error, there are no recover modes for that task, and this feature assumes it has been cancelled before emitting this alert.

How it looks like

Pardon the long video, here it displays triggering the delivery alerts using the API server,

  • warning on a task
  • error on the same task, overwrites the warning (warning disappears)
  • refreshing the dashboard still retrieves the current unresolved delivery alert
  • closing an error delivery alert is considered resolving it (won't show up anymore)
  • different action buttons to change the action field, before the alert is allowed to be closed
simplescreenrecorder-2023-09-20_00.42.09.mp4

Self-checks

  • I have prototyped this new feature (if necessary) on Figma
  • I'm familiar with and follow this Typescript guideline
  • I added unit-tests for new components
  • I tried testing edge cases
  • I tested the behavior of the components that interact with the backend, with an e2e test

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #768 (199f2a0) into deploy/hammer (2be0c73) will decrease coverage by 1.72%.
Report is 6 commits behind head on deploy/hammer.
The diff coverage is 11.97%.

@@                Coverage Diff                @@
##           deploy/hammer     #768      +/-   ##
=================================================
- Coverage          53.71%   51.99%   -1.72%     
=================================================
  Files                263      270       +7     
  Lines               6695     7018     +323     
  Branches             902      958      +56     
=================================================
+ Hits                3596     3649      +53     
- Misses              2958     3230     +272     
+ Partials             141      139       -2     
Flag Coverage Δ
api-server 80.75% <48.54%> (-1.18%) ⬇️
dashboard 15.85% <3.87%> (-1.83%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...rver/api_server/models/tortoise_models/__init__.py 100.00% <100.00%> (ø)
...i_server/models/tortoise_models/delivery_alerts.py 100.00% <100.00%> (ø)
packages/api-server/api_server/rmf_io/__init__.py 100.00% <ø> (ø)
packages/api-server/api_server/rmf_io/events.py 100.00% <100.00%> (ø)
packages/api-server/api_server/routes/__init__.py 100.00% <100.00%> (ø)
packages/dashboard/src/components/app-base.tsx 0.00% <ø> (ø)
packages/dashboard/src/components/appbar.tsx 34.88% <100.00%> (+4.74%) ⬆️
...kages/dashboard/src/components/tasks/tasks-app.tsx 0.00% <ø> (ø)
...ackages/react-components/lib/tasks/create-task.tsx 3.01% <ø> (-0.76%) ⬇️
packages/dashboard/src/hooks/useFetchUser.tsx 50.00% <50.00%> (ø)
... and 7 more

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…ays shown

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…s to display

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth aaronchongth marked this pull request as ready for review September 19, 2023 16:55
Copy link
Collaborator

@Angatupyry Angatupyry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this effort, Aaron!
This was really fast!

I left you minimal suggestions that you can consider.

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth
Copy link
Member Author

Thanks for the review @Angatupyry! I've further simplified the implementation, b082bdf, as we are generating delivery alert IDs using timestamps, we can find out which ones are the newest.

This resolves a bug that I found, where newer warnings don't overwrite older warnings if they are not resolved.

@xiyuoh
Copy link
Member

xiyuoh commented Sep 20, 2023

Some comments/questions after testing onsite:

  • Should Cancel delivery remain as an option? It is currently blacked out
    wrong_cart
  • There shouldn't be an Override option for Missing carts.
    missing_cart_override
  • Probably not related to this PR, but once I've selected System Overview, I can't select other tabs anymore

Copy link
Collaborator

@Angatupyry Angatupyry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

…sing carts

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth
Copy link
Member Author

@xiyuoh

Should Cancel delivery remain as an option? It is currently blacked out

Cancel is only available if the task_id points to a real task, where the task state can be retrieved, otherwise there is no way to cancel it.

The current implementation dispatches the cancellation, we can roll with this for now. If it ever makes things easier on your end, I can remove that, and let you perform the cancellation.

There shouldn't be an Override option for Missing carts.

Good catch, fixed in 003a31b

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth
Copy link
Member Author

@xiyuoh per DM, since updating action and cancelling task is in the same try except scope, the action is not updated if the task cancellation fails, while the button still becomes disabled. Disabling the button should come from feedback on whether the action has been updated instead.

…en properly cancelled

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
…was clicked

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
@aaronchongth
Copy link
Member Author

Merging this to keep the diff small, will apply fixes if needed afterwards.

@aaronchongth aaronchongth merged commit 8add1a6 into deploy/hammer Sep 26, 2023
5 checks passed
@aaronchongth aaronchongth deleted the feature/delivery-alerts branch September 26, 2023 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants